Skip to content

Conversation

nordic-babu
Copy link
Contributor

Extend with no reconfiguration on every tranceive case.

@zephyrbot zephyrbot added area: Tests Issues related to a particular existing or missing test area: SPI SPI bus labels Oct 9, 2025
@nordic-babu nordic-babu force-pushed the extend-spi-error-cases branch 2 times, most recently from a13b59a to 0379236 Compare October 9, 2025 13:37
Extend with no reconfiguration on every tranceive case.

Signed-off-by: Bartlomiej Buczek <[email protected]>
@nordic-babu nordic-babu force-pushed the extend-spi-error-cases branch from 0379236 to 37250eb Compare October 9, 2025 13:51
Copy link

sonarqubecloud bot commented Oct 9, 2025

Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it says anywhere in the SPI API that this would be the behavior. Even if it is a de facto of many drivers. If you want to make this official, it needs to be documented.

@nordic-babu
Copy link
Contributor Author

nordic-babu commented Oct 9, 2025

I do not think it says anywhere in the SPI API that this would be the behavior. Even if it is a de facto of many drivers. If you want to make this official, it needs to be documented.

Please let me know if it's not enough https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/spi.h#L434,

If not I will move this to nordic-specific area for now, this scenario just matched exisitng test suite pretty well for me.

On the other hand frequency of 124999Hz is afaik not documented as invalid anywhere, but such case is present in already existing tests.

@decsny
Copy link
Member

decsny commented Oct 9, 2025

This is a weird case of the API , where the API is not saying how things should be but has a warning about how things may be, so I don't know if it's right to be testing other behavior as an "error". Let's see what @tbursztyka and @teburd think about it

@nordic-babu
Copy link
Contributor Author

on the plus side: at least we'll notice if this behavior changes during some rework and make sure that such a change is intentional (as it requires change in tests).

Copy link
Contributor

@teburd teburd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed this yesterday when I reviewed, this doesn't look right to me

/* change valid config frequency to invalid value */
spim_valid.config.frequency = 124999;

/* make sure device is not reconfigured because conf structure pointer is the same
Copy link
Contributor

@teburd teburd Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite look right to me, why would it not be reconfigured here? Each transceive should be viewed as unique, if you go changing the spi bus frequency between transceives this should be valid, not sure why this wouldn't change the frequency. That's highly confusing behavior.

@teburd
Copy link
Contributor

teburd commented Oct 10, 2025

on the plus side: at least we'll notice if this behavior changes during some rework and make sure that such a change is intentional (as it requires change in tests).

We've gone through this sort of thing before, create the test with the desired/conforming behavior not the existing broken one.

@teburd
Copy link
Contributor

teburd commented Oct 10, 2025

I do not think it says anywhere in the SPI API that this would be the behavior. Even if it is a de facto of many drivers. If you want to make this official, it needs to be documented.

Please let me know if it's not enough https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/spi.h#L434,

If not I will move this to nordic-specific area for now, this scenario just matched exisitng test suite pretty well for me.

On the other hand frequency of 124999Hz is afaik not documented as invalid anywhere, but such case is present in already existing tests.

And this is broken behavior imho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: SPI SPI bus area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants